Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use custom gomega matcher for protobuf equality #4966

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

bestbeforetoday
Copy link
Member

@bestbeforetoday bestbeforetoday commented Aug 27, 2024

The standard gomega Equal matcher is not reliable when used with protobuf messages since they can be logically equal while containing different implementation field values. This change implements a custom ProtoEqual matcher that uses proto.Equal for reliable equality checks.

This implementation enables the following:

Expect(actual).To(ProtoEqual(expected))

which is a more idiomatic alternative to:

Expect(proto.Equal(expected, actual)).To(BeTrue())

In addition, a failed match produces output similar to the following, which is significantly more informative than a message simply stating that false is expected to be true:

[FAILED] Expected
        ChaincodeMessage{
                type:  TRANSACTION
                payload:  "\n\x04arg1\n\x04arg2"
                txid:  "tx-id"
                proposal:  {
                        proposal_bytes:  "signed-proposal-bytes"
                        signature:  "signature"
                }
                channel_id:  "channel-id"
        }
  to equal
        SignedProposal{
                proposal_bytes:  "signed-proposal-bytes"
                signature:  "signature"
        }

@pfi79
Copy link
Contributor

pfi79 commented Aug 27, 2024

@bestbeforetoday Sorry, I see your change is still in the draft. But I'll ask you a question right away.
Will you have a proposal for mixed structures, where there are both proto and non-proto fields.

For example:
https://github.com/hyperledger/fabric/blob/main/core/ledger/kvledger/txmgmt/validation/batch_preparer_test.go#L453
https://github.com/hyperledger/fabric/blob/main/core/ledger/pvtdatastorage/retroactive_hashed_index_test.go#L78

@bestbeforetoday
Copy link
Member Author

@pfi79 Covering both protobuf and non-protobuf elements in a single matcher is definitely possible. It would essentially mean duplicating the behaviour of the existing gomega Equal matcher and extending it to handle protobuf messages using proto.Equal. That is (much) more work than I was planning to tackle in this PR. Instead I was planning just what is implemented here — a matcher for protobuf messages to avoid the need for contributors to remember and use a pattern such as:

Expect(proto.Equal(value1, value2)).To(BeTrue())

The standard gomega Equal matcher is not reliable when used with
protobuf messages since they can be logically equal while containing
different implementation field values. This change implements a custom
ProtoEqual matcher that uses proto.Equal for reliable equality checks.

A failed match produces output similar to the following:

```
[FAILED] Expected
        ChaincodeMessage{
                type:  TRANSACTION
                payload:  "\n\x04arg1\n\x04arg2"
                txid:  "tx-id"
                proposal:  {
                        proposal_bytes:  "signed-proposal-bytes"
                        signature:  "signature"
                }
                channel_id:  "channel-id"
        }
  to equal
        SignedProposal{
                proposal_bytes:  "signed-proposal-bytes"
                signature:  "signature"
        }
```

Signed-off-by: Mark S. Lewis <Mark.S.Lewis@outlook.com>
@bestbeforetoday bestbeforetoday marked this pull request as ready for review August 29, 2024 22:16
@bestbeforetoday bestbeforetoday requested a review from a team as a code owner August 29, 2024 22:16
@bestbeforetoday bestbeforetoday changed the title WIP: Use custom gomega matcher for protobuf equality Use custom gomega matcher for protobuf equality Aug 29, 2024
@andrew-coleman andrew-coleman merged commit 08df37e into hyperledger:main Aug 30, 2024
14 checks passed
@bestbeforetoday bestbeforetoday deleted the protoequal branch August 30, 2024 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants